-
Notifications
You must be signed in to change notification settings - Fork 894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an heuristic to detect/ignore some anomalous TCP ACK packets #1948
Conversation
Still a proof-of-concept. Some notes:
Missing:
|
I selected 3 good traffic examples for testing #1498. |
I quickly tested the traces in your repository and it seems this patch works, IMO. |
Yes! Your version works better. |
@vel21ripn , thanks for the confirmation |
I found another option for incorrect packet processing.
SYN+ACK with length 2 The second packet (syn+ack) gets into the non-empty packet handler.
|
@vel21ripn , are you able to share a pcap with this new pattern? |
I found an even more interesting example of traffic. |
In vel21ripn@6f0c659 there are not SYN-ACK packets with len != 0... |
I added a sample traffic with syn+ack+padding |
Pushed a new version handling the SYN/ACK case |
Even the other 2 traces seems working now |
In some networks, there are some anomalous TCP flows where the smallest ACK packets have some kind of zero padding. It looks like the IP and TCP headers in those frames wrongly consider the 0x00 Ethernet padding bytes as part of the TCP payload. While this kind of packets is perfectly valid per-se, in some conditions they might be treated by the TCP reassembler logic as (partial) overlaps, deceiving the classification engine. Add an heuristic to detect these packets and to ignore them, allowing correct detection/classification. This heuristic is configurable. Default value: * in the library, it is disabled * in `ndpiReader` and in the fuzzers, it is enabled (to ease testing) Credit to @vel21ripn for the initial patch. Close ntop#1946
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! :)
@vel21ripn , what do you think? Can we merge this PR? |
Yes. |
Fix for pre ntop#1948 Fix bytes order for SHA-1 output Changed logic guessed_protocol. We use guessed_protocol_id as app_protocol if it is not excluded. If guessed_protocol_id_by_ip is found, it will be used as app_protocol if not already defined, otherwise it can be used as master_protocol if not already defined.
In some networks, there are some anomalous TCP flows where the smallest
ACK packets have some kind of zero padding.
It looks like the IP and TCP headers in those frames wrongly consider the
0x00 Ethernet padding bytes as part of the TCP payload.
While this kind of packets is perfectly valid per-se, in some conditions
they might be treated by the TCP reassembler logic as (partial) overlaps,
deceiving the classification engine.
Add an heuristic to detect these packets and to ignore them, allowing
correct detection/classification.
This heuristic is configurable. Default value:
ndpiReader
and in the fuzzers, it is enabled (to ease testing)Credit to @vel21ripn for the initial patch.
Close #1946